-
Notifications
You must be signed in to change notification settings - Fork 397
Conversation
@@ -30,7 +31,15 @@ export class GraphqlClient { | |||
); | |||
} | |||
|
|||
if (params.apiVersion) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we do any validation that this is a valid apiVersion
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't usually validate this kind of param - we're trusting TS to shout if anything is wrong, but this can be a version string ("2023-04"
) in pure JS and it will work normally.
@paulomarg since you change the argument type, I am wondering if these aren't breaking changes? It looks like it mostly on internal object but I am not 100% sure |
We're mostly just adding an optional What just occurred to me is that the |
I guess potentially ? I am not sure if we want to go down this route for the few that will be impacted or not. If we apply the rules strictly then yes maybe it should be, but it means waiting for the next major release... |
I agree, IMO we shouldn't hold this change back because of that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following our conversation: you're good to go then
WHY are these changes introduced?
Currently, we receive an
apiVersion
setting when callingshopifyApi()
, and that version is used for any clients throughout the app lifecycle. However, using different versions of the API in different places is a valid use case, for example when trying out a developer preview without migrating everything else.With the way things work now, the clients are unnecessarily rigid and make it harder to interact with multiple versions of the API.
WHAT is this pull request doing?
Adding an
apiVersion
override to all of the clients' constructors, that will be used instead of the global value when present.With this, we can also update the REST resources to internally use their own API version, thus removing the requirement for the app to load the "right" REST resources - with this change, apps can load whatever resources they want to use. We still log a warning if the version mismatches to help remind folks that they might want to update their resources when they change the main version.
Bonus: it seems we'd accidentally removed the REST resource base tests, I'm adding those back. You can ignore those, and the actual changes are here: https://github.com/Shopify/shopify-api-js/pull/660/files/2965e1a9f07ef99e50fd0647f28b17c213dc0476..ea292fbb1fdb4a20213ff9ac54a7144562f71a62
Type of change
Checklist